Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spinner): improve context, action, output, tests #292

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

caarlos0
Copy link
Member

@caarlos0 caarlos0 commented Jun 24, 2024

This PR started as a small one but grew up as I was finding more problems.

  • setting both an action and a context caused the context to be ignored
  • context is now always set if there's any action
  • added ActionWithError(func(context.Context) error), which allows to use the spinner context and return an error, improving error cases usage
  • added Output to set the output, mainly for testing, but might be useful in more places
  • added more tests, many of them testing both accessible and regular flows
  • when running in the regular flow, the action is now run within bubbletea loop (as a tea.Cmd) instead of outside
  • added more spinner examples
  • updated go module to 1.22

@caarlos0 caarlos0 added the enhancement New feature or request label Jun 24, 2024
@caarlos0 caarlos0 self-assigned this Jun 24, 2024
@caarlos0 caarlos0 requested a review from maaslalani as a code owner June 24, 2024 17:59
spinner/spinner.go Outdated Show resolved Hide resolved
spinner/examples/loading/main.go Outdated Show resolved Hide resolved
spinner/spinner.go Outdated Show resolved Hide resolved
spinner/spinner.go Outdated Show resolved Hide resolved
@caarlos0
Copy link
Member Author

another idea would be to have a

type Spinner[T any] struct {
  action func () T
  // ..

and maybe a

type Spinner2[T any, Q any] struct {
  action func() (T, Q)
}

the later being most commonly used as Spinner2[Something, error] I would guess, maybe even worth making the second arg an error always.

@caarlos0
Copy link
Member Author

caarlos0 commented Jun 25, 2024

@maaslalani I reworked some stuff:

  • run the action as part as a tea.Cmd
  • improve handling when having only ctx, only action, both, or neither

@caarlos0
Copy link
Member Author

I think this might also fix #196

@caarlos0
Copy link
Member Author

one bad thing about this, is that it might be a breaking change 🤔

@ccoVeille
Copy link

Can't you support both signature with generics? 🤔

spinner/spinner.go Outdated Show resolved Hide resolved
spinner/spinner.go Outdated Show resolved Hide resolved
Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@caarlos0 caarlos0 changed the title feat(spinner): make action return an error feat(spinner): improve action Jul 25, 2024
@caarlos0
Copy link
Member Author

also added the feature from #172 here

spinner/spinner.go Outdated Show resolved Hide resolved
@caarlos0 caarlos0 requested review from aymanbagabas and bashbunni and removed request for maaslalani August 9, 2024 22:55
spinner/spinner.go Outdated Show resolved Hide resolved
@bashbunni
Copy link
Member

I think I'm missing something obvious here - what's wrong with how we're currently handling actions and how does this change improve that?

@caarlos0
Copy link
Member Author

I think I'm missing something obvious here - what's wrong with how we're currently handling actions and how does this change improve that?

if i recall correctly, if you passed both a context and a function it didn't work properly, that's fixed here

@caarlos0 caarlos0 requested a review from a team as a code owner January 21, 2025 13:09
Copy link
Member

@meowgorithm meowgorithm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, nice work!

spinner/spinner.go Outdated Show resolved Hide resolved
@caarlos0 caarlos0 changed the title feat(spinner): improve action feat(spinner): improve context, action, output, tests Jan 21, 2025
spinner/spinner.go Outdated Show resolved Hide resolved
spinner/spinner.go Outdated Show resolved Hide resolved
spinner/spinner_test.go Outdated Show resolved Hide resolved
caarlos0 and others added 4 commits January 21, 2025 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants